Repository navigation
Preserve live history while persisting a bounded copy - #1796
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1796 +/- ##
=======================================
Coverage 99.75% 99.75%
=======================================
Files 23 23
Lines 6592 6593 +1
=======================================
+ Hits 6576 6577 +1
Misses 16 16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
CI diagnosis: 17/18 OS×Python test cells passed. The macOS 3.15t failure is the first-Ctrl-Z timeout tracked in #1791: I checked the earlier failed run, not just the issue description. It has the same failure signature. The test file has the identical Git blob This supports the existing PTY race diagnosis; it does not make this CI run green or verify #1794. No terminal code/test changes, skips or rerun were made. Codex assisted with this log/source comparison. The current failed test gate remains outstanding. |
tleonhardt
left a comment
tleonhardt
left a comment
There was a problem hiding this comment.
Trim the tests. That's about 100 lines for a 3-line change, and some of it repeats existing tests:
- test_persist_history_no_file_preserves_live_state returns before reaching the changed code, so it can't fail. It also duplicates test_persist_history_ensure_no_error_if_no_histfile.
- test_persist_history_empty also passes with the fix reverted.
- test_persist_history_write_error_preserves_live_state overlaps test_persist_history_permission_error. A live-state assertion could be added to that existing test instead.
- The reload and up-arrow history half of test_persist_history_repeat_append_and_reload mostly re-tests loading, which test_history_populates_pt already covers.
- The parametrized test_persist_history_preserves_live_state is the valuable one. Its limit 4 and 6 cases also add coverage that the saved file holds everything when the limit is at least the history length; nothing tested that before.
|
I merged in main which includes a fix for the one test which previously failed intermittently on free-threaded Python. |
Follow-up to #1796, applying review comments made before it merged. Comment why _persist_history() truncates a copy, so it is not later simplified back into truncating the live history. Trim the new tests. Drop the no-file, empty-history, and write-error cases, which either could not fail or duplicated existing tests, and instead assert unchanged live state in the existing permission-error test with a limit shorter than the history. Drop the reload half of the repeat/append test, which re-tested loading. Simplify the helpers: import lzma directly, move the History import to module level, and stop comparing span and search results that follow from the item identities and session boundary already compared.
Summary
_persist_history()now truncates a temporaryHistorycontainer and serializes it through the existing compressed-JSON path. The live History object, items and session boundary remain intact. Zero/negative limits still save an empty history; order, schema, compression and error reporting are unchanged.The normal current caller is
atexit, where the old truncation is harmless. This addresses the maintainer's direct-call/future-caller concern; it does not add autosave or claim an existing normal-use data-loss incident. No standalone History mutation changes or pending PTY patch are included.Ten regression cases cover shorter/equal/longer/zero/negative limits, empty history, repeated saves and append, absolute/current-session selections, temporary reload and prompt-toolkit history, no filename, and write failure. They compare decoded saved content plus live container/item identities, JSON, session boundary and selections.
Validation
macOS arm64, task-local Python 3.15.2 and locked uv environment:
make checkpassed before Python edits and again after the patch (lock, all prek hooks, ty and mypy). The lockedvalidategroup supplies the existing type stubs;UV_NO_SYNC=1retains that synced group during checks. No lockfile/dependency change.UV_NO_SYNC=1 PYTEST_ADDOPTS='-n 0' make test: 2,848 passed, 3 existing Windows-only skips; coverage options unchanged, total 99.5%. All 31 pipeline job-control tests passed in this run.UV_NO_SYNC=1 make docs-test: strict build passed, no issues.git diff --checkpassed.Codex assisted with implementation, tests and this description. I reviewed the diff and ran these checks; no independent human review is claimed. All history, files, runtime and caches used for proof were task-owned. No personal history/configuration was used. Linux/Windows and other supported Python versions remain for upstream CI. No Markdown/spec/plan or release changes are included; normal exit behavior and the documented public API are unchanged.
Closes #1789.